Fixes #38460 - Disable CCV auto-publish on child CV minor version publish.#11629
Fixes #38460 - Disable CCV auto-publish on child CV minor version publish.#11629qcjames53 merged 1 commit intoKatello:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When building
propagated_composite_cv_idsinContentView::IncrementalUpdates#plan, consider calling.uniq(and possibly.compact) on the mapped IDs to avoid passing duplicates or nils through to the auto-publish exclusion logic. - The new tests rely on stubbing
ContentViewVersion.find(nil)and other internals of the action; using a concreteold_versionID instead ofnilwould make the tests less coupled to the implementation and more resilient to future refactors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When building `propagated_composite_cv_ids` in `ContentView::IncrementalUpdates#plan`, consider calling `.uniq` (and possibly `.compact`) on the mapped IDs to avoid passing duplicates or nils through to the auto-publish exclusion logic.
- The new tests rely on stubbing `ContentViewVersion.find(nil)` and other internals of the action; using a concrete `old_version` ID instead of `nil` would make the tests less coupled to the implementation and more resilient to future refactors.
## Individual Comments
### Comment 1
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:20` </location>
<code_context>
+ let(:component_cv) { katello_content_views(:library_dev_view) }
+ let(:component_version) { component_cv.versions.first }
+ let(:composite_cv) { katello_content_views(:composite_view) }
+ let(:mock_output) { {} }
+
+ before do
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting `auto_publish_content_view_version_id` in addition to `auto_publish_content_view_ids`
Right now the tests only verify the filtering of `auto_publish_content_view_ids`, but not the behavior of `auto_publish_content_view_version_id`, which is also updated in the implementation.
In addition to the existing assertions, please:
- In a positive case (e.g., `includes composites in auto-publish list when not propagated`), assert `mock_output[:auto_publish_content_view_version_id] == component_version.id`.
- In negative cases (composite CV, non-latest version), assert that `mock_output[:auto_publish_content_view_version_id]` is `nil` or absent, matching the expectations for `auto_publish_content_view_ids`.
This will ensure the tests fully cover the auto-publish output contract.
Suggested implementation:
```ruby
it 'includes composites in auto-publish list when not propagated' do
# existing setup / execution
perform_action action, input
assert_equal [composite_cv.id], mock_output[:auto_publish_content_view_ids]
assert_equal component_version.id, mock_output[:auto_publish_content_view_version_id]
end
```
```ruby
it 'does not include composite cv in auto-publish list when composite cv is the target' do
# existing setup / execution
perform_action action, input
assert_empty mock_output[:auto_publish_content_view_ids]
assert_nil mock_output[:auto_publish_content_view_version_id]
end
```
```ruby
it 'does not include composite in auto-publish list when component version is not latest' do
# existing setup / execution
perform_action action, input
assert_empty mock_output[:auto_publish_content_view_ids]
assert_nil mock_output[:auto_publish_content_view_version_id]
end
```
I assumed the test names and assertion shapes based on your description. To wire this into your actual file:
1. Locate the example that currently asserts `mock_output[:auto_publish_content_view_ids]` and whose description matches “includes composites in auto-publish list when not propagated”; add `assert_equal component_version.id, mock_output[:auto_publish_content_view_version_id]` right after the existing assertion.
2. For each “negative” example where you assert an empty/absent `auto_publish_content_view_ids` (e.g. composite CV as target, non-latest version cases), add `assert_nil mock_output[:auto_publish_content_view_version_id]` (or the equivalent expectation style you are using) next to the existing `mock_output[:auto_publish_content_view_ids]` assertion.
3. If your test suite uses `test "..." do` instead of `it '...' do`, or uses `refute`/`wont_be_nil` instead of `assert_nil`, adapt the snippets accordingly while keeping the semantics: positive case → equals `component_version.id`, negative cases → `nil`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
IncrementalUpdateTest, the stubContentViewVersion.stubs(:find).with(nil)coupled with not settingold_versionin the input makes the test rely on an implicitnilargument; consider explicitly settingold_version(or avoiding thefind(nil)path) so the tests are less brittle against future changes in the action inputs. - The test case
handles partial propagation with multiple compositesfakes a second composite ID viacomposite_cv.id + 100; it would be more robust to create a real second composite content view (with a corresponding version) so the test reflects realistic data and avoids relying on ID arithmetic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `IncrementalUpdateTest`, the stub `ContentViewVersion.stubs(:find).with(nil)` coupled with not setting `old_version` in the input makes the test rely on an implicit `nil` argument; consider explicitly setting `old_version` (or avoiding the `find(nil)` path) so the tests are less brittle against future changes in the action inputs.
- The test case `handles partial propagation with multiple composites` fakes a second composite ID via `composite_cv.id + 100`; it would be more robust to create a real second composite content view (with a corresponding version) so the test reflects realistic data and avoids relying on ID arithmetic.
## Individual Comments
### Comment 1
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:9-10` </location>
<code_context>
+ include Support::Actions::Fixtures
+ include FactoryBot::Syntax::Methods
+
+ before :all do
+ User.current = users(:admin)
+ end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Prefer per-test setup to avoid leaking User.current across test cases
Using `before :all` here shares `User.current` across examples and can leak state as tests are added or cleanup changes. Please use per-example setup (e.g., `before` or `setup`) so each test runs with its own explicitly set `User.current` and doesn’t depend on cross-test state.
</issue_to_address>
### Comment 2
<location> `test/actions/katello/content_view_version/incremental_update_test.rb:41-43` </location>
<code_context>
+ # Stub repositories to avoid the content comparison logic in run
+ component_version.stubs(:repositories).returns([])
+
+ # Stub find for old_version (will be nil in most tests, stub to return a version with empty repos)
+ old_version_stub = stub('old_version', repositories: [])
+ ::Katello::ContentViewVersion.stubs(:find).with(nil).returns(old_version_stub)
+ end
+
</code_context>
<issue_to_address>
**nitpick:** Remove or narrow the stub for ContentViewVersion.find(nil) if it’s no longer needed
This stub is described as for `old_version`, but `run` only calls `ContentViewVersion.find` with `input[:new_content_view_version_id]`, and each example stubs that explicitly. The `find(nil)` stub may never be used. If it isn’t, please remove it; if it is, add a brief comment or assertion indicating which code path requires `find(nil)` so it’s clear to future readers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
test/actions/katello/content_view_version/incremental_update_test.rb
Outdated
Show resolved
Hide resolved
test/actions/katello/content_view_version/incremental_update_test.rb
Outdated
Show resolved
Hide resolved
test/actions/katello/content_view_version/incremental_update_test.rb
Outdated
Show resolved
Hide resolved
test/actions/katello/content_view_version/incremental_update_test.rb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @qcjames53!
LGTM! Verified fix works correctly and all tests pass 👍
bash-5.1$ hammer content-view version incremental-update --content-view-version-id 108 --errata-ids 43357 --propagate-all-composites true
[......................................................................................................................................] [100%]
Content View: ChildCV version 1.1
Added Content:
Content View: ParentCCV version 1.1
Added Content:
bash-5.1$ hammer content-view version list --content-view "ParentCCV" --organization "Default Organization"
----|---------------|---------|-------------|-----------------------
ID | NAME | VERSION | DESCRIPTION | LIFECYCLE ENVIRONMENTS
----|---------------|---------|-------------|-----------------------
111 | ParentCCV 1.1 | 1.1 | | Library
109 | ParentCCV 1.0 | 1.0 | |
----|---------------|---------|-------------|-----------------------
pavanshekar
left a comment
There was a problem hiding this comment.
The CI test failure in Actions::Katello::ContentView::IncrementalUpdatesTest#test_0001_plans is because the existing test needs to expect the new parameter.
Please update the test expectations in test/actions/katello/content_view_test.rb:
Line 953-954 - Add :propagated_composite_cv_ids => []:
assert_action_planned_with(action, ::Actions::Katello::ContentViewVersion::IncrementalUpdate, content_view.version(library), [library],
:content => {:errata_ids => ["FOO"]}, :resolve_dependencies => true, :description => "BadDescription", :propagated_composite_cv_ids => [])Line 968-969 - Add :propagated_composite_cv_ids => [composite_version.content_view_id]:
assert_action_planned_with(action, ::Actions::Katello::ContentViewVersion::IncrementalUpdate, component, [],
:content => {:errata_ids => ["FOO"]}, :resolve_dependencies => true, :description => "BadDescription", :propagated_composite_cv_ids => [composite_version.content_view_id])|
I've fixed the failing tests. Thanks. |
|
I think this change may require an adjustment to BATS: https://ci.theforeman.org/job/katello-nightly-rpm-pipeline/2685/tapResults/ Caught it since I've been keeping an eye on the results after redoing auto publish |
|
Thanks for the heads up, Jonathon. Zach just took care of this BATS issue in this PR: theforeman/forklift#1932 |
What are the changes introduced in this pull request?
When publishing a minor version of a content view (in the testing steps achieved through an incremental update applying errata), parent composite content views no longer respect the
--propagate-all-compositesflag. This avoids a duplicate publish where a content view publishes a minor version, updates the parent composite content view, then propagates itself to the composite again.Considerations taken when implementing this change?
The unit tests were made with a ton of AI help. Please double-check everything is reasonable.
What are the testing steps for this pull request?
Summary by Sourcery
Prevent duplicate auto-publishing of composite content views when performing incremental updates on child content view versions with propagate enabled.
Bug Fixes:
Tests: